Respect pagination when querying a workspace dataframe with sql param - #859
Merged
Conversation
`repositories::workspaces::data_frames::query` dropped its `DFOpts` on the branch that runs caller-supplied SQL, passing `None` to `sql::query_df` where the other branch passes `Some(opts)`. `opts` is what carries page/page_size, so a read with a `sql` param returned the whole result set for every page. `prepare_sql` now composes the page onto the statement instead of appending to it, since caller SQL can carry its own ORDER BY, LIMIT, or OFFSET: - A statement that bounds its own extent is paged through a subquery, so the two bounds nest rather than collide, and no sort of ours reorders the rows it already picked. - Otherwise the statement's own ORDER BY wins over `opts.sort_by`. - A paginated statement left with no order at all gets `ORDER BY _oxen_row_id` wherever that column binds — the same stable order the non-sql branch builds, so an edited row stays on its page. Statements that aggregate, dedupe, or read a derived table can't bind it and page in whatever order they produce. The GET handler counts what the query selects rather than what the frame holds, so `total_pages` and `total_entries` describe the same row set the page came out of. `PyWorkspaceDataFrame::sql_query` still returns the full result, now by reading the pages. It stops on a short page, which is both the end of the result and what a server too old to paginate the query answers page 1 with.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds SQL-aware row counting, preserves query-owned ordering and bounds, applies safe pagination, moves indexed operations to blocking tasks, and retrieves Python SQL results in one maximum-sized page with explicit validation. ChangesSQL Pagination and Data-Frame Querying
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PythonClient
participant DataFrameController
participant WorkspaceDataFrames
participant DuckDB
PythonClient->>DataFrameController: Request SQL results
DataFrameController->>WorkspaceDataFrames: Count and execute SQL with DFOpts
WorkspaceDataFrames->>DuckDB: Prepare and run paginated SQL
DuckDB-->>WorkspaceDataFrames: Rows and selected-result count
WorkspaceDataFrames-->>DataFrameController: Page data and total
DataFrameController-->>PythonClient: Validated result response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/liboxen/src/core/db/data_frames/df_db.rs`:
- Around line 520-528: Update count_sql to normalize the caller’s SQL before
embedding it in the derived-table query: parse exactly one statement and remove
its terminal semicolon/delimiter, then compose the count query and downstream
pagination SQL from the normalized statement. Add coverage for
semicolon-terminated SQL while preserving existing behavior for delimiter-free
input.
In `@crates/oxen-server/src/controllers/workspaces/data_frames.rs`:
- Around line 199-214: In the data-frame handler’s count/query flow, split the
combined tasks::spawn_blocking closure into two operations: one closure that
performs repositories::workspaces::data_frames::count_for_query and one that
performs query. Await and propagate errors from each independently while
preserving the existing count and dataframe results.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe3d00db-de08-4232-bdc6-b5c1651129e8
📒 Files selected for processing (5)
crates/liboxen/src/core/db/data_frames/df_db.rscrates/liboxen/src/core/v_latest/data_frames.rscrates/liboxen/src/repositories/workspaces/data_frames.rscrates/oxen-py/src/py_workspace_data_frame.rscrates/oxen-server/src/controllers/workspaces/data_frames.rs
`prepare_sql` and `count_sql` place a caller's statement somewhere other than the end of the text: before an appended `ORDER BY` or `LIMIT`, or inside a count's derived table. A trailing `;` or `--` comment is harmless at the end of a statement and changes what follows it in the middle — the terminator makes the composed statement a syntax error, and a comment swallows the bounds so the read returns the whole frame instead of one page. `add_special_columns` re-renders through the parser only when it injects `_oxen_id`, and returns the statement as written otherwise: for a DISTINCT, for a projection that isn't a subset of the source schema, and for one that already selects `_oxen_id`, which `SELECT *` does because the column is really there. So the text reaching composition is often the caller's own. Route both composition sites through `composable` to render a single parsed statement instead. Text that doesn't parse as exactly one statement passes through unchanged, for DuckDB to reject as it did before.
The workspace data frame read moved both DuckDB calls off the request thread in one closure. docs/async_policy.md puts the granularity at one offload per operation, and specifically not one bespoke closure per handler: sharing a closure means neither call can overlap with the other or be converted to an async API on its own. Give each its own offload. Both already open their own connection, so nothing was shared but the hop.
jcelliott
marked this pull request as ready for review
August 12, 2026 15:04
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/liboxen/src/core/db/data_frames/df_db.rs`:
- Around line 539-543: Update composable to catch Parser::parse_sql errors and
return Ok(sql.to_string()) instead of propagating them; continue serializing
only when exactly one statement parses successfully, while returning the
original SQL for zero or multiple statements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad386fcb-c35b-4800-b82f-5a36443bb269
📒 Files selected for processing (3)
crates/liboxen/src/core/db/data_frames/df_db.rscrates/liboxen/src/repositories/workspaces/data_frames.rscrates/oxen-server/src/controllers/workspaces/data_frames.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/liboxen/src/core/db/data_frames/df_db.rs (1)
531-544: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
composabledoes not implement its documented parse-error fallback.The doc comment states that text which does not parse as exactly one statement is returned as written, so the database rejects it on its own terms. The implementation propagates
Parser::parse_sqlerrors with?instead. Only the multi-statement and zero-statement cases fall back to the raw text. Either returnOk(sql.to_string())on a parse error, or correct the doc to state that unparsable text is rejected bysqlparser. TheDIALECTisPostgreSqlDialect, so DuckDB-specific syntax thatsqlparsercannot parse now fails with a parser error.♻️ Option: match the documented behavior
fn composable(sql: &str) -> Result<String, DataFrameError> { - match Parser::parse_sql(&DIALECT, sql)?.as_slice() { - [stmt] => Ok(stmt.to_string()), - _ => Ok(sql.to_string()), - } + match Parser::parse_sql(&DIALECT, sql) { + Ok(stmts) => match stmts.as_slice() { + [stmt] => Ok(stmt.to_string()), + _ => Ok(sql.to_string()), + }, + Err(_) => Ok(sql.to_string()), + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/liboxen/src/core/db/data_frames/df_db.rs` around lines 531 - 544, Update composable to catch Parser::parse_sql errors and return Ok(sql.to_string()), preserving the documented raw-text fallback for unparsable input while retaining the existing fallback for zero or multiple statements.
🔇 Additional comments (10)
crates/liboxen/src/core/db/data_frames/df_db.rs (2)
594-607: LGTM!Also applies to: 616-658
660-675: LGTM!Also applies to: 677-731
crates/liboxen/src/repositories/workspaces/data_frames.rs (3)
123-138: LGTM!
164-179: 🗄️ Data Integrity & Integration
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that no caller paginates the result of
querya second time.
querynow appliesopts.pageandopts.page_sizeinside SQL. A caller that also slices or paginates the returnedDataFramewith the sameoptswould apply the page twice and return the wrong rows. The server controller usesfrom_df_and_opts_unpaginated, so it is safe. Confirm the remaining callers, includingexportand any client-facing paths, do not re-apply pagination.
2442-2457: LGTM!Also applies to: 2459-2525, 2527-2578, 2580-2610, 2612-2655, 2657-2683
crates/liboxen/src/core/v_latest/data_frames.rs (1)
165-167: LGTM!crates/oxen-server/src/controllers/workspaces/data_frames.rs (3)
198-220: LGTM!
1395-1403: 🎯 Functional Correctness
⚠️ Unverified finding
Sandbox verification was unavailable.Verify that
to_http_query_paramsemitspage=0andpage_size=0.
test_get_unindexed_data_frame_paginatessendspage_query(0, 10, None)andpage_query(1, 0, None)to assert the handler clamps both values to 1. IfDFOpts::to_http_query_paramsskips zero or default values, the request carries nopage_size, the handler appliesDEFAULT_PAGE_SIZE, and the clamp assertions pass for the wrong reason.
1474-1516: LGTM!Also applies to: 1526-1620, 1635-1688
crates/oxen-py/src/py_workspace_data_frame.rs (1)
186-192: LGTM!Also applies to: 236-241
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/oxen-py/src/py_workspace_data_frame.rs`:
- Around line 194-235: Document the pagination constraint in the Python API
surrounding the multi-page query flow: results spanning more than PAGE_SIZE rows
require a caller-supplied ORDER BY for stable pagination, particularly for
DISTINCT, grouped, or derived-table queries. Alternatively, detect the absence
of caller ordering before accumulating multiple pages and return an OxenError
rather than silently returning duplicated or missing rows.
---
Duplicate comments:
In `@crates/liboxen/src/core/db/data_frames/df_db.rs`:
- Around line 531-544: Update composable to catch Parser::parse_sql errors and
return Ok(sql.to_string()), preserving the documented raw-text fallback for
unparsable input while retaining the existing fallback for zero or multiple
statements.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29e24fbb-aa02-4a09-8e0d-339ac452a579
📒 Files selected for processing (5)
crates/liboxen/src/core/db/data_frames/df_db.rscrates/liboxen/src/core/v_latest/data_frames.rscrates/liboxen/src/repositories/workspaces/data_frames.rscrates/oxen-py/src/py_workspace_data_frame.rscrates/oxen-server/src/controllers/workspaces/data_frames.rs
The unindexed branch derives a slice from the requested page, and `slice_indices` reads those bounds back as i64. A page_size only usize can hold overflows that parse, and the parse panics rather than erroring, so a request naming a page wider than i64 takes the server down to a 500 instead of reading a page. Narrow page_size to what the bounds can carry when deriving the slice. The widest page a request can name then reads as the whole frame, which is what a client asking for one oversized page means by it. `slice_indices` still panics on bounds it cannot parse, reachable through `slice` directly; that expect is worth removing on its own terms.
`sql_query` returns every row a query selects, and read the paginated endpoint a page at a time to collect them. Pages of one query are not pages of one result: the read orders a query by `_oxen_row_id` only where that column resolves against it, so a query that groups or dedupes and carries no `ORDER BY` of its own need not return rows in the same order twice. Stitching its pages together repeats some rows and drops others, and answers the query with neither an error nor its result. Ask for the whole result as a single page instead, which is what this returns either way — the Python `query(sql=...)` ignores a page number, and `get_embeddings` reads every row a query matches.
gschoeni
self-requested a review
August 12, 2026 21:23
gschoeni
approved these changes
Aug 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
repositories::workspaces::data_frames::querydropped itsDFOptson the branch that runs caller-supplied SQL, passingNonetosql::query_dfwhere the other branch passesSome(opts).optsis what carries page/page_size, so a read with asqlparam returned the whole result set for every page.prepare_sqlnow composes the page onto the statement instead of appending to it, since caller SQL can carry its own ORDER BY, LIMIT, or OFFSET.